Create GetInsightData for deploy frequency by sanposhiho · Pull Request #1188 · pipe-cd/pipecd

sanposhiho · 2020-12-04T03:54:02Z

What this PR does / why we need it:

Create GetInsightData for deploy frequency

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

pipecd-bot

Some issues were detected while linting go source files in your changes.

pipecd-bot · 2020-12-04T03:54:54Z

pkg/app/api/grpcapi/web_api.go

 }
+
+// getInsightDataForDeployFrequency returns the accumulated insight data for deploy frequency.
+func (a *WebAPI) getInsightDataForDeployFrequency(ctx context.Context, projectId string, req *webservice.GetInsightDataRequest) (*webservice.GetInsightDataResponse, error) {


method parameter projectId should be projectID

https://golang.org/wiki/CodeReviewComments#initialisms

pipecd-bot · 2020-12-04T04:02:22Z

Code coverage for golang is 33.48%. This pull request increases coverage by 0.20%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.getInsightDataForDeployFrequency	`--`	`93.33%`	`+93.33%`
pkg/app/api/grpcapi/web_api.go	WebAPI.GetInsightData	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2020-12-04T04:18:07Z

Code coverage for golang is 33.48%. This pull request increases coverage by 0.20%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.getInsightDataForDeployFrequency	`--`	`93.33%`	`+93.33%`
pkg/app/api/grpcapi/web_api.go	WebAPI.GetInsightData	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2020-12-04T04:49:07Z

Code coverage for golang is 33.50%. This pull request increases coverage by 0.23%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.getInsightDataForDeployFrequency	`--`	`100.00%`	`+100.00%`
pkg/app/api/grpcapi/web_api.go	WebAPI.GetInsightData	`0.00%`	`0.00%`	`+0.00%`

cakecatz · 2020-12-04T04:49:42Z

/lgtm

pkg/app/api/grpcapi/web_api.go

nakabonne · 2020-12-04T07:09:54Z

pkg/app/api/grpcapi/web_api_test.go

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+


nit: can you remove the unneeded line?

nakabonne · 2020-12-04T07:29:34Z

Thanks for the beautiful code! Looks good except for some nits comments. I especially like the part of modularizing movePoint func.

sanposhiho · 2020-12-04T07:39:33Z

@nakabonne Thank you for your review, fixed them :)

nakabonne · 2020-12-04T07:41:31Z

/cc @nghialv

nakabonne · 2020-12-04T07:41:43Z

/lgtm

pipecd-bot · 2020-12-04T10:33:38Z

The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by /golinter trigger command right now.

You can check the build log from here.

pipecd-bot · 2020-12-04T10:39:22Z

Code coverage for golang is 33.46%. This pull request increases coverage by 0.18%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.getInsightDataForDeployFrequency	`--`	`96.15%`	`+96.15%`
pkg/app/api/grpcapi/web_api.go	WebAPI.GetInsightData	`0.00%`	`0.00%`	`+0.00%`

sanposhiho · 2020-12-04T10:42:39Z

@nghialv Thank you for your review, fixed them :)

pipecd-bot · 2020-12-04T11:13:38Z

Code coverage for golang is 33.46%. This pull request increases coverage by 0.19%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.getInsightDataForDeployFrequency	`--`	`92.86%`	`+92.86%`
pkg/app/api/grpcapi/web_api.go	WebAPI.GetInsightData	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2020-12-06T21:51:23Z

The following files are not gofmt-ed. By commenting /golinter fmt, the formatted one will be appended to this pull request automatically.

pkg/app/api/grpcapi/web_api.go

--- pkg/app/api/grpcapi/web_api.go.orig
+++ pkg/app/api/grpcapi/web_api.go
@@ -1382,7 +1382,7 @@
 		for j := 0; ; j++ {
 			deployments, err := a.deploymentStore.ListDeployments(ctx, datastore.ListOptions{
 				PageSize: pageSize,
-				Page: j + 1,
+				Page:     j + 1,
 				Filters:  filters,
 			})
 			if err != nil {

pkg/app/api/grpcapi/web_api_test.go

--- pkg/app/api/grpcapi/web_api_test.go.orig
+++ pkg/app/api/grpcapi/web_api_test.go
@@ -421,7 +421,7 @@
 				s.EXPECT().
 					ListDeployments(gomock.Any(), datastore.ListOptions{
 						PageSize: PageSizeForListDeployments,
-						Page: 1,
+						Page:     1,
 						Filters: []datastore.ListFilter{
 							{
 								Field:    "ProjectId",
@@ -458,7 +458,7 @@
 				s.EXPECT().
 					ListDeployments(gomock.Any(), datastore.ListOptions{
 						PageSize: PageSizeForListDeployments,
-						Page: 1,
+						Page:     1,
 						Filters: []datastore.ListFilter{
 							{
 								Field:    "ProjectId",
@@ -528,7 +528,7 @@
 				s.EXPECT().
 					ListDeployments(gomock.Any(), datastore.ListOptions{
 						PageSize: PageSizeForListDeployments,
-						Page: 1,
+						Page:     1,
 						Filters: []datastore.ListFilter{
 							{
 								Field:    "ProjectId",

pipecd-bot · 2020-12-06T22:01:23Z

Code coverage for golang is 33.46%. This pull request increases coverage by 0.19%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.getInsightDataForDeployFrequency	`--`	`92.86%`	`+92.86%`
pkg/app/api/grpcapi/web_api.go	WebAPI.GetInsightData	`0.00%`	`0.00%`	`+0.00%`

nghialv · 2020-12-07T02:38:35Z

pkg/app/api/grpcapi/web_api_test.go

+	"time"
+
+	"go.uber.org/zap"



nit: Remove this empty line. (zap is a third-party package so it should be at the same group with gomock and assert.)

nghialv · 2020-12-07T06:39:21Z

Nice. Thank you.
/approve

pipecd-bot · 2020-12-07T06:39:24Z

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

sanposhiho · 2020-12-07T06:39:41Z

@nghialv
cc @nakabonne

Remove Page from the list option and add a comment about temporary implementation.

nakabonne · 2020-12-07T06:40:52Z

pkg/app/api/grpcapi/web_api.go

 }

 // getInsightDataForDeployFrequency returns the accumulated insight data for deploy frequency.
+// This function is temporary implementation for front end.


sanposhiho added 2 commits December 4, 2020 12:52

Create GetInsightData for deploy frequency

a34eb41

Add test for getInsightDataForDeployFrequency

79393b0

pipecd-bot added release-note-none area/api size/L labels Dec 4, 2020

pipecd-bot requested review from cakecatz and nghialv December 4, 2020 03:54

sanposhiho marked this pull request as draft December 4, 2020 03:54

pipecd-bot reviewed Dec 4, 2020

View reviewed changes

Rename projectId to projectID

6d0a32a

Add assert in test for response

4344548

pipecd-bot added size/XL and removed size/L labels Dec 4, 2020

sanposhiho marked this pull request as ready for review December 4, 2020 04:12

Add test to check error pattern

54d2c7a

pipecd-bot added the lgtm label Dec 4, 2020

nakabonne reviewed Dec 4, 2020

View reviewed changes

pkg/app/api/grpcapi/web_api.go Outdated Show resolved Hide resolved

nakabonne reviewed Dec 4, 2020

View reviewed changes

sanposhiho added 2 commits December 4, 2020 16:32

Remove empty line

6c1965f

Fix to catch invalid step

8681e87

pipecd-bot removed the lgtm label Dec 4, 2020

pipecd-bot added the lgtm label Dec 4, 2020

sanposhiho added 6 commits December 4, 2020 19:09

Fix to use zap.NewNop()

74cb3bd

Rename accumulateFrom to start

efd3f84

Fix to specify timezone UTC

79061bd

Fix to reset to zero in weekly and daily

651c401

Add pageSize opts

4939e23

Support only daily to prevent heavy loading

fbbd84c

pipecd-bot added size/L and removed lgtm size/XL labels Dec 4, 2020

Remove unuse deps

bb63a36

Add comment for support

127d2a3

Fix to specify page on ListDeployments option

33abb45

Go fmted

e19da30

nghialv reviewed Dec 7, 2020

View reviewed changes

sanposhiho added 2 commits December 7, 2020 15:34

Fix not to get all data with page

98c92e7

Remove empty line

06e9695

pipecd-bot added the approved label Dec 7, 2020

nakabonne reviewed Dec 7, 2020

View reviewed changes

pipecd-bot merged commit 7696c88 into master Dec 7, 2020

pipecd-bot deleted the create-rpc-deployment-frequency branch December 7, 2020 06:46

pipecd-bot mentioned this pull request Dec 16, 2020

Add doc for pipectl #1266

Merged


		for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {

Conversation

sanposhiho commented Dec 4, 2020

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

pipecd-bot Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Dec 4, 2020

Uh oh!

pipecd-bot commented Dec 4, 2020

Uh oh!

pipecd-bot commented Dec 4, 2020

Uh oh!

cakecatz commented Dec 4, 2020

Uh oh!

Uh oh!

nakabonne Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

nakabonne commented Dec 4, 2020

Uh oh!

sanposhiho commented Dec 4, 2020

Uh oh!

nakabonne commented Dec 4, 2020

Uh oh!

nakabonne commented Dec 4, 2020

Uh oh!

pipecd-bot commented Dec 4, 2020

Uh oh!

pipecd-bot commented Dec 4, 2020

Uh oh!

sanposhiho commented Dec 4, 2020

Uh oh!

pipecd-bot commented Dec 4, 2020

Uh oh!

pipecd-bot commented Dec 6, 2020

Uh oh!

pipecd-bot commented Dec 6, 2020

Uh oh!

nghialv Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

nghialv commented Dec 7, 2020

Uh oh!

pipecd-bot commented Dec 7, 2020

Uh oh!

sanposhiho commented Dec 7, 2020

Uh oh!

nakabonne Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants